Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for forwarding stdin data to server, and getting stdout and stderr from server #4

Closed
wants to merge 4 commits into from

Conversation

sergiou87
Copy link
Member

This PR introduces a few changes:

  1. The trampoline reads info passed via stdin and forwards it to the server.
  2. The server might send more data back to the trampoline, which will have to output in stdout and stderr.
  3. Added tests for all the new things.
  4. Refactor in those tests.

* Reads a string from the socket, reading first 2 bytes to get its length and
* then the string itself.
*/
ssize_t readDelimitedString(SOCKET socket, char *buffer, size_t bufferSize) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is basically used to read the stdout and stderr messages sent from the server.

Comment on lines +53 to +63
uint16_t outputLength = 0;
if (readSocket(socket, &outputLength, sizeof(uint16_t)) < (int)sizeof(uint16_t)) {
printSocketError("ERROR: Error reading from socket");
return -1;
}

if (outputLength > bufferSize) {
fprintf(stderr, "ERROR: received string is bigger than buffer (%d > %zu)", outputLength, bufferSize);
return -1;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads the length of the stdout/stderr data to be received. It's encoded in 2 bytes (uint16_t).

Comment on lines +64 to +80
size_t totalBytesRead = 0;
ssize_t bytesRead = 0;

// Read output from server
do {
bytesRead = readSocket(socket, buffer + totalBytesRead, outputLength - totalBytesRead);

if (bytesRead == -1) {
printSocketError("ERROR: Error reading from socket");
return -1;
}

totalBytesRead += bytesRead;
} while (bytesRead > 0);

buffer[totalBytesRead] = '\0';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads the actual data, and appends a \0 at the end.

const buffer = Buffer.alloc(2, 0)

// Send stdout
buffer.writeUInt16LE(stdout.length, 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeUInt16LE assumes the trampoline (and tests) will always run in little-endian machines. In the unlikely scenario where we support big-endian architectures, we should revisit this (or the trampoline, to make sure it always reads the bytes in the right order).

// Don't send anything and just close the socket after the trampoline is
// done forwarding data.
socket.end()
async function configureTrampolineServer(stdout = '', stderr = '') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this cannot be a beforeEach function (or I didn't see how), since we need to pass parameters when the server is created 😞

Comment on lines +117 to +133
const process = execFile(trampolinePath, ['baz'], opts, function (
err,
stdout,
stderr
) {
if (!err) {
resolve({ stdout, stderr, exitCode: 0 })
return
}

reject(err)
})

process.stdin.end(
'This is a test\nWith a multiline\nStandard input text',
'utf-8'
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this promise dance is needed to inject data to the trampoline via stdin.

@sergiou87
Copy link
Member Author

Another thing to consider: right now the buffer length is 4KB, which means that will be the limit of the commit message (+ some additional info like author and ancestors) for commit signing. Maybe we should bump that a bit?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't finished fully setting up for windows

@niik
Copy link
Member

niik commented Jun 11, 2024

Superseded by #36

@niik niik closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants